-
Notifications
You must be signed in to change notification settings - Fork 116
Update openvm to 1.4.0 #3291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update openvm to 1.4.0 #3291
Conversation
…f, but can't get column names
[Ovm 1.4] get original airs
[OVM 1.4] `VmConfig` related trait implementations
Should we have this merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
||
[features] | ||
default = [] | ||
tco = ["openvm-sdk/tco"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's tco?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tail call optimization
repository.workspace = true | ||
|
||
[features] | ||
default = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is metrics
default in cli-openvm
but not here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the metrics are required for benchmarks to work on ci. Happy to remove the default and pass the feature explicitly there. For here, same, I don't have a strong opinion, but metrics sounds more like something you'd turn on only if needed.
|
||
fn pc_step(&self) -> u32 { | ||
self.0.step | ||
DEFAULT_PC_STEP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This relies on different behavior as before. Do we not have access to the program's step anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, as far as I understand, only the default is supported now.
&exe.program, | ||
labels.clone(), | ||
exe.program.pc_base, | ||
exe.program.step, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct
new_local_prover(vm_builder, &vm_pk, exe)?; | ||
|
||
vm_instance.reset_state(inputs.clone()); | ||
let metered_ctx = vm_instance.vm.build_metered_ctx(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to do all this new stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was inlined from on internal function in ovm to build the proving context without calling the prover, and then run debug_proving_ctx
. Agree it could be cleaner.
air_by_opcode_id.air_name_to_machine.get(air_name).unwrap(); | ||
|
||
// TODO: main_columns might not be correct, as the RA::with_capacity() uses the following `main_width()` | ||
// pub fn main_width(&self) -> usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's with this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR updated the openvm dependency to 1.4.0. A lot of changes were introduced, hence the size of this PR.
Not covered in this PR:
Changes to openvm here
Changes to stark-backend here
Tasks:
Send + Sync
bound on Executor in ovm (instead, removed the need for that bound in ovm)